gh-128341: Simplify the definition of _Py_INTERNAL_ABI_SLOT and use it in stdlib modules#145770
gh-128341: Simplify the definition of _Py_INTERNAL_ABI_SLOT and use it in stdlib modules#145770befeleme wants to merge 2 commits intopython:mainfrom
Conversation
_PyABIInfo_DEFAULT is available in Include/modsupport.h, there's not need to duplicate the definition here. Rename from _Py_INTERNAL_ABI_SLOT, as it's no longer internal.
This enables running a check of ABI version compatibility. _tkinter, _tracemalloc and readline don't use the slots, hence they need explicit handling.
befeleme
requested review from
1st1,
AA-Turner,
AlexWaygood,
FFY00,
JelleZijlstra,
StanFromIreland,
abalkin,
ammaraskar,
asvetlov,
berkerpeksag,
brettcannon,
corona10,
emmatyping,
ericsnowcurrently,
erlend-aasland,
gpshead,
kumaraditya303,
lysnikolaou,
ncoghlan,
pablogsal,
pganssle,
picnixz,
rhettinger,
serhiy-storchaka,
warsaw and
willingc
as code owners
| } | ||
|
|
||
| static struct PyModuleDef_Slot module_slots[] = { | ||
| _Py_ABI_SLOT, |
There was a problem hiding this comment.
_asyncio is a builtin module as it is statically linked, this check is not needed as it is not a shared library module.
There was a problem hiding this comment.
It's possible to configure Python to build the _asyncio extension as a shared library:
$ cat Modules/Setup.local
*shared*
_asyncio _asynciomodule.c
$ ./python
>>> import _asyncio
>>> _asyncio
<module '_asyncio' from '/home/vstinner/python/main/build/lib.linux-x86_64-3.15/_asyncio.cpython-315d-x86_64-linux-gnu.so'>
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
| PyABIInfo_VAR(abi_info); | ||
|
|
||
| static struct PyModuleDef_Slot module_slots[] = { | ||
| _Py_ABI_SLOT, |
There was a problem hiding this comment.
With this change, there are two Py_mod_abi slots. I'm not sure that it makes sense. Either remove {Py_mod_abi, &abi_info} and the associated abi_info, or remove _Py_ABI_SLOT.
Same remark for Modules/_testcapimodule.c.
| #endif | ||
| // For internal use in stdlib. Needs C99 compound literals. | ||
| // Defined here to avoid every stdlib module including pycore_modsupport.h | ||
| #define _Py_ABI_SLOT {Py_mod_abi, (void*) &(PyABIInfo) _PyABIInfo_DEFAULT} |
There was a problem hiding this comment.
I don't understand the (void*) &(PyABIInfo) part. Where is this variable declared? Or does it magically accept the PyABIInfo type here?
I would prefer keeping _Py_INTERNAL_ABI_SLOT name to discourage developers to use it in their project.
Defined here to avoid every stdlib module including pycore_modsupport.h
If it's technically possible to use a macro in a C extension, someone will do it and then it will be hard for us to change the macro (users always complain).
4 participants
Please consider discussing this in the issue linked, rather than it the PR, since this would ping a huge list of reviewers.